Improve text returned when checking VFS availability.

- Display more clear error when adding local sync folder and when selecting remote folder.
- Modernize FolderWizardRemotePath::isComplete().
- Display the local folder path when warning that folder is already being synced.

Signed-off-by: Camila Ayres <hello@camilasan.com>

diff --git a/src/common/vfs.cpp b/src/common/vfs.cpp
index 4f22bfedb0dec2e1d86f77945b76da9992945c5a..1d0b6d23da7ce3ae5b0ffa6332c4eafd55031802 100644 (file)
--- a/src/common/vfs.cpp
+++ b/src/common/vfs.cpp
@@ -71,16 +71,16 @@ Result<void, QString> Vfs::checkAvailability(const QString &path, Vfs::Mode mode
 #ifdef Q_OS_WIN
     if (mode == Mode::WindowsCfApi) {
         const auto info = QFileInfo(path);
-        if (QDir(info.canonicalFilePath()).isRoot()) {
-            return tr("The Virtual filesystem feature does not support a drive as sync root");
+        if (QDir(info.canonicalPath()).isRoot()) {
+            return tr("Please choose a different location. %1 is a drive. It doesn't support virtual files.").arg(path);
         }
-        const auto fs = FileSystem::fileSystemForPath(info.absoluteFilePath());
-        if (fs != QLatin1String("NTFS")) {
-            return tr("The Virtual filesystem feature requires a NTFS file system, %1 is using %2").arg(path, fs);
+        if (const auto fileSystemForPath = FileSystem::fileSystemForPath(info.absoluteFilePath());
+            fileSystemForPath != QLatin1String("NTFS")) {
+            return tr("Please choose a different location. %1 isn't a NTFS file system. It doesn't support virtual files.").arg(path);
         }
         const auto type = GetDriveTypeW(reinterpret_cast<const wchar_t *>(QDir::toNativeSeparators(info.absoluteFilePath().mid(0, 3)).utf16()));
         if (type == DRIVE_REMOTE) {
-            return tr("The Virtual filesystem feature is not supported on network drives");
+            return tr("Please choose a different location. %1 is a network drive. It doesn't support virtual files.").arg(path);
         }
     }
 #else

diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp
index 126a39d5cf97ae844dac1392c74483363c1f25b8..fb9427c7d284f1f46a0fd4311feca3d7aa268721 100644 (file)
--- a/src/gui/folder.cpp
+++ b/src/gui/folder.cpp
@@ -197,11 +197,11 @@ bool Folder::checkLocalPath()
         QString error;
         // Check directory again
         if (!FileSystem::fileExists(_definition.localPath, fi)) {
-            error = tr("Local folder %1 does not exist.").arg(_definition.localPath);
+            error = tr("Please choose a different location. The folder %1 doesn't exist.").arg(_definition.localPath);
         } else if (!fi.isDir()) {
-            error = tr("%1 should be a folder but is not.").arg(_definition.localPath);
+            error = tr("Please choose a different location. %1 isn't a valid folder.").arg(_definition.localPath);
         } else if (!fi.isReadable()) {
-            error = tr("%1 is not readable.").arg(_definition.localPath);
+            error = tr("Please choose a different location. %1 isn't a readable folder.").arg(_definition.localPath);
         }
         if (!error.isEmpty()) {
             _syncResult.appendErrorString(error);

diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp
index ff84383c526958493d7fa087adfde5af6f43b140..69c7dfeab836e2b715e45bd918dd1104b9391d2d 100644 (file)
--- a/src/gui/folderman.cpp
+++ b/src/gui/folderman.cpp
@@ -1799,7 +1799,7 @@ QString FolderMan::trayTooltipStatusString(SyncResult::Status syncStatus, bool h
 static QString checkPathValidityRecursive(const QString &path)
 {
     if (path.isEmpty()) {
-        return FolderMan::tr("No valid folder selected!");
+        return FolderMan::tr("Please choose a different location. The selected folder isn't valid.");
     }
 
 #ifdef Q_OS_WIN
@@ -1807,18 +1807,20 @@ static QString checkPathValidityRecursive(const QString &path)
 #endif
     const QFileInfo selFile(path);
     if (numberOfSyncJournals(selFile.filePath()) != 0) {
-        return FolderMan::tr("The folder %1 is used in a folder sync connection!").arg(QDir::toNativeSeparators(selFile.filePath()));
+        return FolderMan::tr("Please choose a different location. %1 is already being used as a sync folder.").arg(QDir::toNativeSeparators(selFile.filePath()));
     }
 
     if (!FileSystem::fileExists(path)) {
         QString parentPath = selFile.dir().path();
-        if (parentPath != path)
+        if (parentPath != path) {
             return checkPathValidityRecursive(parentPath);
-        return FolderMan::tr("The selected path does not exist!");
+        }
+
+        return FolderMan::tr("Please choose a different location. The path %1 doesn't exist.").arg(QDir::toNativeSeparators(selFile.filePath()));
     }
 
     if (!FileSystem::isDir(path)) {
-        return FolderMan::tr("The selected path is not a folder!");
+        return FolderMan::tr("Please choose a different location. The path %1 isn't a folder.").arg(QDir::toNativeSeparators(selFile.filePath()));
     }
 
     #ifdef Q_OS_WIN
@@ -1826,12 +1828,12 @@ static QString checkPathValidityRecursive(const QString &path)
         // isWritable() doesn't cover all NTFS permissions
         // try creating and removing a test file, and make sure it is excluded from sync
         if (!Utility::canCreateFileInPath(path)) {
-            return FolderMan::tr("You have no permission to write to the selected folder!");
+            return FolderMan::tr("Please choose a different location. You don't have enough permissions to write to %1.", "folder location").arg(QDir::toNativeSeparators(selFile.filePath()));
         }
     }
     #else
     if (!FileSystem::isWritable(path)) {
-        return FolderMan::tr("You have no permission to write to the selected folder!");
+        return FolderMan::tr("Please choose a different location. You don't have enough permissions to write to %1.", "folder location").arg(QDir::toNativeSeparators(selFile.filePath()));
     }
     #endif
     return {};
@@ -1884,17 +1886,15 @@ QPair<FolderMan::PathValidityResult, QString> FolderMan::checkPathValidityForNew
         bool differentPaths = QString::compare(folderDir, userDir, cs) != 0;
         if (differentPaths && folderDir.startsWith(userDir, cs)) {
             result.first = FolderMan::PathValidityResult::ErrorContainsFolder;
-            result.second = tr("The local folder %1 already contains a folder used in a folder sync connection. "
-                              "Please pick another one!")
+            result.second = tr("Please choose a different location. %1 is already being used as a sync folder.")
                                 .arg(QDir::toNativeSeparators(path));
             return result;
         }
 
         if (differentPaths && userDir.startsWith(folderDir, cs)) {
             result.first = FolderMan::PathValidityResult::ErrorContainedInFolder;
-            result.second = tr("The local folder %1 is already contained in a folder used in a folder sync connection. "
-                      "Please pick another one!")
-                .arg(QDir::toNativeSeparators(path));
+            result.second = tr("Please choose a different location. %1 is already contained in a folder used as a sync folder.")
+                                .arg(QDir::toNativeSeparators(path));
             return result;
         }
 
@@ -1908,8 +1908,9 @@ QPair<FolderMan::PathValidityResult, QString> FolderMan::checkPathValidityForNew
 
             if (serverUrl == folderUrl) {
                 result.first = FolderMan::PathValidityResult::ErrorNonEmptyFolder;
-                result.second = tr("There is already a sync from the server to this local folder. "
-                          "Please pick another local folder!");
+                result.second = tr("Please choose a different location. %1 is already being used as a sync folder for %2.", "folder location, server url")
+                                    .arg(QDir::toNativeSeparators(path),
+                                         serverUrl.toString());
                 return result;
             }
         }

diff --git a/src/gui/folderwizard.cpp b/src/gui/folderwizard.cpp
index 6a45da8a15aeda1514772bd3aa8131deb5e16a9f..3561699d610eed8c1304781f85cb3d2700bc2205 100644 (file)
--- a/src/gui/folderwizard.cpp
+++ b/src/gui/folderwizard.cpp
@@ -61,18 +61,18 @@ namespace OCC {
 
 QString FormatWarningsWizardPage::formatWarnings(const QStringList &warnings) const
 {
-    QString ret;
+    QString formattedWarning;
     if (warnings.count() == 1) {
-        ret = tr("<b>Warning:</b> %1").arg(warnings.first());
+        formattedWarning = warnings.first();
     } else if (warnings.count() > 1) {
-        ret = tr("<b>Warning:</b>") + " <ul>";
-        Q_FOREACH (QString warning, warnings) {
-            ret += QString::fromLatin1("<li>%1</li>").arg(warning);
+        formattedWarning = "<ul>";
+        for (const auto &warning : warnings) {
+            formattedWarning += QString::fromLatin1("<li>%1</li>").arg(warning);
         }
-        ret += "</ul>";
+        formattedWarning += "</ul>";
     }
 
-    return ret;
+    return formattedWarning;
 }
 
 FolderWizardLocalPath::FolderWizardLocalPath(const AccountPtr &account)
@@ -484,34 +484,39 @@ FolderWizardRemotePath::~FolderWizardRemotePath() = default;
 
 bool FolderWizardRemotePath::isComplete() const
 {
-    if (!_ui.folderTreeWidget->currentItem())
+    if (!_ui.folderTreeWidget->currentItem()) {
         return false;
+    }
 
-    QStringList warnStrings;
-    QString dir = _ui.folderTreeWidget->currentItem()->data(0, Qt::UserRole).toString();
-    if (!dir.startsWith(QLatin1Char('/'))) {
-        dir.prepend(QLatin1Char('/'));
+    auto targetPath = _ui.folderTreeWidget->currentItem()->data(0, Qt::UserRole).toString();
+    if (!targetPath.startsWith(QLatin1Char('/'))) {
+        targetPath.prepend(QLatin1Char('/'));
     }
-    wizard()->setProperty("targetPath", dir);
+    wizard()->setProperty("targetPath", targetPath);
 
-    Folder::Map map = FolderMan::instance()->map();
-    Folder::Map::const_iterator i = map.constBegin();
-    for (i = map.constBegin(); i != map.constEnd(); i++) {
-        auto *f = static_cast<Folder *>(i.value());
-        if (f->accountState()->account() != _account) {
+    for (const auto folder : std::as_const(FolderMan::instance()->map())) {
+        if (folder->accountState()->account() != _account) {
             continue;
         }
-        QString curDir = f->remotePathTrailingSlash();
-        if (QDir::cleanPath(dir) == QDir::cleanPath(curDir)) {
-            warnStrings.append(tr("This folder is already being synced."));
-        } else if (dir.startsWith(curDir)) {
-            warnStrings.append(tr("You are already syncing <i>%1</i>, which is a parent folder of <i>%2</i>.").arg(Utility::escape(curDir), Utility::escape(dir)));
-        } else if (curDir.startsWith(dir)) {
-            warnStrings.append(tr("You are already syncing <i>%1</i>, which is a subfolder of <i>%2</i>.").arg(Utility::escape(curDir), Utility::escape(dir)));
+
+        const auto remoteDir = folder->remotePathTrailingSlash();
+        const auto localDir = folder->cleanPath();
+        if (QDir::cleanPath(targetPath) == QDir::cleanPath(remoteDir)) {
+            showWarn(tr("Please choose a different location. %1 is already being synced to %2.").arg(Utility::escape(remoteDir), Utility::escape(localDir)));
+            break;
+        }
+
+        if (targetPath.startsWith(remoteDir)) {
+            showWarn(tr("Please choose a different location. %1 is already being synced to %2.").arg(Utility::escape(targetPath), Utility::escape(localDir)));
+            break;
+        }
+
+        if (remoteDir.startsWith(targetPath)) {
+            showWarn(tr("Please choose a different location. %1 is already being synced to %2.").arg(Utility::escape(remoteDir), Utility::escape(localDir)));
+            break;
         }
     }
 
-    showWarn(formatWarnings(warnStrings));
     return true;
 }
 
@@ -625,7 +630,10 @@ bool FolderWizardSelectiveSync::validatePage()
     if (useVirtualFiles) {
         const auto availability = Vfs::checkAvailability(wizard()->field(QStringLiteral("sourceFolder")).toString(), mode);
         if (!availability) {
-            auto msg = new QMessageBox(QMessageBox::Warning, tr("Virtual files are not available for the selected folder"), availability.error(), QMessageBox::Ok, this);
+            auto msg = new QMessageBox(QMessageBox::Warning,
+                                       tr("Virtual files are not supported at the selected location"),
+                                       availability.error(),
+                                       QMessageBox::Ok, this);
             msg->setAttribute(Qt::WA_DeleteOnClose);
             msg->open();
             return false;

diff --git a/src/gui/wizard/owncloudadvancedsetuppage.cpp b/src/gui/wizard/owncloudadvancedsetuppage.cpp
index f515e1c716a65072c2fb94acd801b7b4922e9464..bb51e447438d0680061fed24a098353cd587450d 100644 (file)
--- a/src/gui/wizard/owncloudadvancedsetuppage.cpp
+++ b/src/gui/wizard/owncloudadvancedsetuppage.cpp
@@ -395,7 +395,10 @@ bool OwncloudAdvancedSetupPage::validatePage()
     if (useVirtualFileSync()) {
         const auto availability = Vfs::checkAvailability(localFolder(), bestAvailableVfsMode());
         if (!availability) {
-            auto msg = new QMessageBox(QMessageBox::Warning, tr("Virtual files are not available for the selected folder"), availability.error(), QMessageBox::Ok, this);
+            auto msg = new QMessageBox(QMessageBox::Warning,
+                                       tr("Virtual files are not supported at the selected location"),
+                                       availability.error(),
+                                       QMessageBox::Ok, this);
             msg->setAttribute(Qt::WA_DeleteOnClose);
             msg->open();
             return false;

diff --git a/test/testfolderman.cpp b/test/testfolderman.cpp
index f40c3c7e4e1c619a1223abd651965241c5b9aa8d..c837378281c2470f7e5fae17c93370f47608b95e 100644 (file)
--- a/test/testfolderman.cpp
+++ b/test/testfolderman.cpp
@@ -313,8 +313,8 @@ private slots:
         // The following both fail because they are already sync folders even if for another account
         QUrl url3("http://anotherexample.org");
         url3.setUserName("dummy");
-        QCOMPARE(folderman->checkPathValidityForNewFolder(dirPath + "/sub/ownCloud1", url3).second, QString("The folder %1 is used in a folder sync connection!").arg(QDir::toNativeSeparators(dirPath + "/sub/ownCloud1")));
-        QCOMPARE(folderman->checkPathValidityForNewFolder(dirPath + "/ownCloud2", url3).second, QString("The folder %1 is used in a folder sync connection!").arg(QDir::toNativeSeparators(dirPath + "/ownCloud2")));
+        QCOMPARE(folderman->checkPathValidityForNewFolder(dirPath + "/sub/ownCloud1", url3).second, QString("Please choose a different location. %1 is already being used as a sync folder.").arg(QDir::toNativeSeparators(dirPath + "/sub/ownCloud1")));
+        QCOMPARE(folderman->checkPathValidityForNewFolder(dirPath + "/ownCloud2", url3).second, QString("Please choose a different location. %1 is already being used as a sync folder.").arg(QDir::toNativeSeparators(dirPath + "/ownCloud2")));
 
         QVERIFY(!folderman->checkPathValidityForNewFolder(dirPath).second.isNull());
         QVERIFY(!folderman->checkPathValidityForNewFolder(dirPath + "/sub/ownCloud1/folder").second.isNull());
@@ -337,7 +337,7 @@ private slots:
         // link 3 points to an existing sync folder. To make it fail, the account must be the same
         QVERIFY(!folderman->checkPathValidityForNewFolder(dirPath + "/link3", url2).second.isNull());
         // while with a different account, this is fine
-        QCOMPARE(folderman->checkPathValidityForNewFolder(dirPath + "/link3", url3).second, QString("The folder %1 is used in a folder sync connection!").arg(dirPath + "/link3"));
+        QCOMPARE(folderman->checkPathValidityForNewFolder(dirPath + "/link3", url3).second, QString("Please choose a different location. %1 is already being used as a sync folder.").arg(dirPath + "/link3"));
 
         QVERIFY(!folderman->checkPathValidityForNewFolder(dirPath + "/link4").second.isNull());
         QVERIFY(!folderman->checkPathValidityForNewFolder(dirPath + "/link3/folder").second.isNull());